-
-
Notifications
You must be signed in to change notification settings - Fork 577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
modified search metadata to return Astropy.Table #3950
Conversation
CHANGELOG.rst
Outdated
@@ -196,6 +196,7 @@ Trivial/Internal Changes | |||
- Corrected spelling of 'plotting' in timeseries method (changed 'ploting' to 'plotting'). (`#3429 <https://github.com/sunpy/sunpy/pull/3429>`__) | |||
- Switched to "importlib_metadata" to get package version to speed up import of SunPy. (`#3449 <https://github.com/sunpy/sunpy/pull/3449>`__) | |||
- Fix tests for `sunpy.data.data_manager` and ensure they are correctly executed with pytest. (`#3550 <https://github.com/sunpy/sunpy/pull/3550>`__) | |||
- Modified the `search_metadata` function inside `sunpy.net.jsoc` to return the Astropy.Table instead of pandas dataframe. (`#3950 <https://github.com/sunpy/sunpy/pull/3950>`__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use this changelog directly. You have to add an RST file into the changelogs folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nabobalis should I remove the change in CHANGELOG and only upload a file in changes.rst? or do we have to keep both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this change and add a changelog file in to the changelog folder.
What does the output look like now in terminal compared with before? |
New Output
Old output
|
Looking at that, I do think we should keep the first column from the old version. |
first column is actually the index value, @nabobalis should I create a column named index in the astropy.Table and put index-values in it and do the indexing according to them? |
Sure. |
Co-Authored-By: Nabil Freij <nabil.freij@gmail.com>
Hello @DShivansh! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-03-31 20:08:33 UTC |
What does the output look like now? |
output
|
As things stand, this PR is good to go but we need to resolve how to deal with the breaking change or even if we need this function anymore. These two questions are unresolved right now and as such, we can't merge this PR until then. |
@nabobalis should we raise some type of warning that indicates the user that we don't return pandas dataframe anymore instead it is returning the astropy df. |
The warning needs to have a way to turn it off and warning the user as they run the code after a update isn't really an advanced warning. |
can we use some kind of flag in sunpyrc to turn off the warning once user knows about it? |
I don't have a good solution right now. |
The way I have seen this sort of deprecation done previously is to add a |
I think with the current GSoC project, we will probably remove this method as a result I will be closing this. I want to thank @DShivansh for opening the PR and I am sorry we decided to change the goal posts. |
Fixes #3941